-
Notifications
You must be signed in to change notification settings - Fork 2
implement mergeVariableStates! #1145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1145 +/- ##
===========================================
+ Coverage 72.52% 72.61% +0.09%
===========================================
Files 29 29
Lines 2428 2436 +8
===========================================
+ Hits 1761 1769 +8
Misses 667 667 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
return sum(cnt) | ||
end | ||
|
||
function mergeVariableStates!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have expected the default implementation signature to be
function mergeState!(<:Variable, <:VariableState)
# actual implementation lives here
end
mergeState!(dfg::Factorgraph, lb::Symbol, state::VariableState) = mergeState!(getVariable(fg, lb), state)
mergeStates!(dfg::Factorgraph, ::Vector{<:Pair_}) = # pmap over vector mergeState!(dfg, pair[1], pair[2])
mergeVariableState!(w...;kw...) = mergeState!(w...;kw...) # alias
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mergeState!
is not consistent with the rest of the API as we have getVariableState
and therefore would expect mergeVariableState!
. We need the Variable
noun in get
explicit to avoid ambiguities, to have a type stable implementation in julia, and to help with SDKs without multiple dispatch. It also makes for more readable and maintainable code to be more explicit -- when updating from getSolverData
, it was a pain to distinguish between Variable and Factor. We can still do the alias, but that would likely be a DFG only function and not across SDKs. I would also not include [get/add/merge/delete]State[!]
for DFGv1 beta work.
Does the comment ... ::Vector{<:Pair_})
mean you are happy with the new signature, what I wanted to know form this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also see similar discussion here on possible future expansion of FactorState.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with ::Vector{Pair{Symbol, <:VariableState}}
. There might be some dynamic dispatch if different Variable states are merged, but that is okay too in Julialand -- AND, we are fixing many of those cases separately with the HomotopyBelief* idea:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EDIT: Should add that the signature with implementation vs alias does not really bother the DFG v1 release -- this is only where the functional code actually resides. Both getState
and getVariableState
are still valid according to the DFG v1 spec. Similar for mergeState!
vs mergeVariableState!
.
Don't loose sight of
therefore would expect
You expect the longer signature, however, I expect zero duplication is better. For getVariableState(::VariableDataType)
"variable" noun twice.
not consistent with the rest of the API
Again, that is what I was trying to warn about in issue 56. Obviously the overall philosophy of how to build signatures dominates everything BUT I'm not sure we are on the same page. Here is the top line description from the verbNoun wiki (which has been my mental picture all along):
verbNoun[Noun|Adjective][!](::NounType[,..., defaults=defaults; kwargs=defaults])
Shorter is better. The longer verbNounNoun(::NounType,...)
is opposite to what I understood we had decided before. I thought we are building the shortest possible name in Julia's dispatch and then adding the longer signatures as aliases. For non-dispatch/non-overloading language SDKs we only have the longer signatures (and the core implementations are in Julia only anyway).
Do you have a reference/issue/comment somewhere on changing this default philosophy to longer (w/ noun duplication) rather than shorter names (as per wiki)?
it was a pain to distinguish between Variable and Factor.
I get you were struggling during refactor, but you have to look at this in hindsight (after DFG v1 has landed). You struggled because the old code was a mess. Your struggle was hopefully the last time the verbNoun refac needs to go through such as major cleanup.
Personally, I do not see a problem with using either: getState(::Variable_,...)
or getState(:Factor_,...)
and these would return different types. That's the key, Julia's ability to return different types. Julia implementations would live in getState
(similar to old getSolver[d]ata
), with aliases such as getFactorState
.
mergeState!
[...vs...]mergeVariableState!
.
mergeState!
is pretty natural for me. Inconsistent with rest of API and struggled during refactor sound like transient reasons, and non-dispatch SDKs are basically just shims (glorified aliases) over GQL. Is there perhaps another reason for more verbose?
I do remember we discussed choice between getStateVariable
vs getVariableState
-- again, I was under the impression the decision from there is to only implement getState
and then include alias getVariableState
. In extreme cases maybe e as far as including a second alias getStateVariable
(bad example).
It also makes for more readable and maintainable code to be more explicit
Proper use of multiple dispatch reduces maintenance. Verbose manual write out of what could have been dispatch increases maintenance load.
Less duplication is more readable for me, because the explicit "Variable" in function name tells me there are cases without variables. I think we should use multiple dispatch properly and only add the longer aliases to have consistency with other non-dispatch SDK languages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also just cross referencing NavAbility/NavAbilitySDK.jl#288, where getVariableState
was approved as API. We should change that also pending this outcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exists
is another similar case. Variables and factors use just exists
and all other nouns use hasObj
. (The verb noun wiki says we are going with has
, so just giving as an example)
The signature is just exists(fg, label)
so we need to read the documentation that it will look for variable or factor existence where hasVaraible and hasFactor is a more self explanatory. What about [graph|agent]blobentries/metadata, etc. existence? With has... autocomplete gets you there. We can consider keeping exists
as a DFG function that calls to has
internally. That is not "implement the logic inside the shorthand method definition", but I don't really see the value in that as a hard rule and more implementation specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edit: technically we can just have add!, but that is not the case, so I don't think that hase been the design so far. We briefly discussed this i a call and decided to go with addVariable! for dfgv1.
I remember from this discussion that logic would go in the shorthand and alias for the longer signatures to match other languages that cannot dispatch.
Do you have a reference/issue/comment somewhere on changing this default philosophy to longer (w/ noun duplication) rather than shorter names (as per wiki)?
Is this decision written somewhere so i can follow the reasoning.
yep, I'm asking the same question.
Personally, I do not see a problem with using either: getState(::Variable_,...) or getState(:Factor_,...) and these would return different types. [skipping your reply as vague but will continue from the phone call conversation that was better]
I previously missed your point that dispatch, although near infinite, is not always type stable. So getState(::AbstractDFG, ::Symbol)
could return either FactorState
or VariableState
from the same signature -- hence require slow dynamic dispatch. This for me is a good reason for a more verbose signature. HOWEVER, the shorter signature for getState(::AbstractVariable / ::AbstractFactor)
should still produce type stable code and therefore good. Only the highest level API calls may be a problem for short signatures, but lower down should be good.
Also just cross referencing NavAbility/NavAbilitySDK.jl#288, where getVariableState was approved as API. We should change that also pending this outcome.
From phone call, the answer is likely DFG v1.x where shorter signatures are added or just logic moved around, but no breaking changes. Sounds like most important is just getting the verb nouns consistent all over as necessary step -- i.e. focus on that for now.
mergeVariablesVariablestates!
This is exactly why I favor the short names with dispatch and don't want to handicap Julia dispatch design for the sake of the other languages. So I would add the verbose signatures as aliases over short dispatches like just add!
or merge!
-- essence of good design once we get there (will take time to get that clean a design though).
For this case I would just go with mergeVariableStates!
:
function mergeVariableStates!(
::AbstractDFG,
::Symbol,
::Vector{<:Pair{Symbol, <:VariableState}},
)
getVarstate
andgetFacstate
I don't like these short names. In that case better to go with longer getVariableState
or getFactorState
.
copilot's opinion:
Nope, I disagree with copilot but thanks for checking and copying that over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exists is another similar case. Variables and factors use just exists and all other nouns use hasObj. (The verb noun wiki says we are going with has, so just giving as an example)
Sounds like the verbNoun wiki/dictionary is wrong. While has
and exist
may both be there, the definitions should make it clear which one is the best.
We can consider keeping exists as a DFG function that calls to has internally.
depends on what the best definitions for exist
and has
are. If the definitions are bad then I suggest you fix/change/update the definitions first and then update the code second. Don't just follow the dictionary if it doesn't make sense, change the dictionary.
The signature is just exists(fg, label) so we need to read the documentation that it will look for variable or factor. What about [graph|agent]blobentries/metadata, etc. existence? With has... autocomplete gets you there.
Again, I would expect to just find exists
and tab complete for 100's of dispatches with a single clear docustring, and a few related examples (but limited to stable types per signature). Pretty sure that is what core Julia is doing already. I'd be okay to compromise for now with always searching both variable and factors and rather preserve the short signature exists(fg, lbl)
, while avoiding dynamic types..
We can consider keeping exists as a DFG function that calls to has internally. That is not "implement the logic inside the shorthand method definition", but I don't really see the value in that as a hard rule and more implementation specific.
on this I would again just say that when the design is perfect, then only a few high level dispatches would cover everything, but that quality design is going to take time and instead solve under DFG v1.x guidelines -- i.e. make the decision that avoids breaking changes during DFG v1.x series releases and that allows the signatures to become shorter and more Julia dispatchy as the minor releases roll.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mergeVariablesVariablestates!
This is exactly why I favor the short names with dispatch and don't want to handicap Julia dispatch design for the sake of the other languages. So I would add the verbose signatures as aliases over short dispatches like just add! or merge! -- essence of good design once we get there (will take time to get that clean a design though).
I don't like these short names. In that case better to go with longer getVariableState or getFactorState.
and other replies...
I think you are still missing the issue here. The current nouns are VariableState
and FactorState
not just State
- that breaks the capital rule as with BlobEntry vs Blobentry -- but this as a rule might be a mistake
- That causes the shorthand to be
getVariableState
and the long version to begetVariableVariableState
So, let me ask directly in another way, how would you like to distinguish between VariableState
and FactorState
structs?
Keep in mind that that will be the Noun used in CRUD.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Note, branch was rebased to resolve conflics. |
Just noting differentiating between the accessor-like
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see topic by topic responses
I went back and forth on
mergeVariableStates!
and comparing it withupdateVariableNodeData!
and settled on following the verb by still providing a container. The same concept can be followed for all satellite levels (such asBlobentry
)@dehann would you mind giving your inputs.